Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix GH#26593: Crash when importing MusicXML with multiple fermatas on a grace note #26602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Feb 19, 2025

Resolves: #26593

applies to master and 4.5.0

@cbjeukendrup
Copy link
Contributor

I think it would be better to look for the root cause of the problem, rather than hiding it. When a Fermata has no Segment, it probably means that either the parent is nullptr (which is invalid anyway) or that the parent has only been passed to the constructor and there has been no setParent call to make explicitParent() return something.

But for fermatas in particular, there also seems to be a special situation where they are first temporarily added to a ChordRest (which becomes its parent), and then moved to a Segment. See ChordRest::addFermata. Perhaps that plays a role here. But that would be unexpected too, since that temporary situation is supposed to be resolved during the MusicXML import, so should not be seen during layout.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 22, 2025

Well, ChordRest::addFermata looks like this:

    //! TODO Look like a hack, see using
    void addFermata(Fermata* f) { addEl(f); }
    void removeFermata(Fermata* f) { removeEl(f); }
    //! --------------------------------

It is used only once in importmusicxmlpass2.cpp:

    Fermata* na = Factory::createFermata(cr);
...
    if (cr->segment() == nullptr && cr->isGrace()) {
        cr->addFermata(na);           // store for later move to segment
    } else {
        cr->segment()->add(na);
    }

When crashing, the FERMATA in question does have a(n explict) parent (that is not explicitly set ?!?) and that parent is a CHORD. That chord apparently is a grace note, as the call is comming from here:

void ChordLayout::layoutPitched(Chord* item, LayoutContext& ctx)
{
    for (Chord* c : item->graceNotes()) {
        layoutPitched(c, ctx);
    }

So it indeed must have gone through that code (the addFermata()) mentioned above.

addFermata() calls addEl(), which in turn is only used there and for STRECHED_BEND, CHORDLINE and FRET_CIRCLE ?!?

I do see that adding a fermata to a grace note really adds it to the parent note/chord, I don't see however where this move happens?

@Jojo-Schmitz Jojo-Schmitz changed the title Fix GH#26593: Crash when opening a specific MusicXML score Fix GH#26593: Crash when importing MusicXML with a fermata on a grace note Feb 22, 2025
@Jojo-Schmitz Jojo-Schmitz changed the title Fix GH#26593: Crash when importing MusicXML with a fermata on a grace note Fix GH#26593: Crash when importing MusicXML with multiple fermatas on a grace note Feb 22, 2025
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 22, 2025

The problem seems to be that there are multiple fermatas added to gracenote in the .musicxml file:

      <note default-x="367.51" default-y="15.00">
        <grace/>
        <pitch>
          <step>B</step>
          <octave>5</octave>
          </pitch>
        <voice>1</voice>
        <type>16th</type>
        <stem>up</stem>
        <staff>1</staff>
        <beam number="1">begin</beam>
        <beam number="2">begin</beam>
        <notations>
          <fermata type="upright"/>
          <fermata type="upright"/>
          <fermata type="upright"/>
          <fermata type="upright"/>
          </notations>
        </note>
 

Removing those duplicates apparently fixes the crash

The pretty bad part is that the exporter was MuseScore 3.4.2... The score stems from http://musescore.com/score/658031 BTW

Minimized (manually stripped down) sample score: foo.zip, 3 grace notes with 4 fermatas each. But 3.6.2 imports only 7 (!?) of them:
image
Even stranger: when removing the duplicates 3.6.2 still shows 7!
And 4.x doesn't crash anymore but shows 7 too, only superimposed on one another, so it at least looks right.

@Jojo-Schmitz Jojo-Schmitz force-pushed the musicxml-fermata-crash branch 2 times, most recently from 9ba0cdb to a2077f2 Compare February 22, 2025 13:16
@Jojo-Schmitz Jojo-Schmitz force-pushed the musicxml-fermata-crash branch from a2077f2 to 1a10093 Compare February 22, 2025 13:52
cbjeukendrup added a commit to cbjeukendrup/MuseScore that referenced this pull request Feb 22, 2025
…rd to segment

Resolves: musescore#26593

If a MusicXML file contains multiple fermatas on one grace note, then those fermatas except the first would not be moved from the grace chord to the segment. This means that there are fermatas with no segment encountered during layout when going through that grace chord's `el()`, which causes a crash.

Inspired by the investigation at musescore#26602.
@cbjeukendrup
Copy link
Contributor

See #26680 for an alternative solution, partially inspired by the comments here

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Feb 23, 2025

Looks good to me (but see my comment there), although mine might get used in addition, as an extra safety net? Checking for nullptr before dereferencing usn't wrong after all.
Or mine for 4.5 (being way smaller and less risky?), (mine and) your's for 4.6?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segmentation fault when opening a specific MusicXML score
2 participants